feat(rp): Add UnauthorizedHandler #503
Conversation
Signed-off-by: Jan-Otto Kröpke <[email protected]>
pkg/client/rp/relying_party.go
Outdated
| // UnauthorizedHandler returns the handler used for unauthorized errors | ||
| UnauthorizedHandler() func(http.ResponseWriter, *http.Request, string, string) | ||
|
|
There was a problem hiding this comment.
Adding a method to the interface would be a breaking change. For the proposed implementation we don't need to expose the method, right?
There was a problem hiding this comment.
Correct, we do not need to expose the method. however CodeExchangeHandler is using the interface as input. Without declare the function on the interface, the method is not available.
oidc/pkg/client/rp/relying_party.go
Line 448 in 2b35eeb
As I mention, I would prefer to re-use the existing rp.ErrorHandler, but it does not fit in this case here.
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #503 +/- ##
==========================================
- Coverage 60.12% 59.98% -0.14%
==========================================
Files 80 80
Lines 6962 6978 +16
==========================================
Hits 4186 4186
- Misses 2480 2496 +16
Partials 296 296 ☔ View full report in Codecov by Sentry. |
|
@muhlemmer do you an alternative solution here? If I had the choice, I would think about an breaking change which modifies the function signature of the existing ErrorHandler by accepting the http status as input parameter and use the ErrorHandler on all places. I think this is more convenient as having two functions. |
|
For this PR to the main branch you could use an optional interface: type HasUnauthorizedHandler interface {
// UnauthorizedHandler returns the handler used for unauthorized errors
UnauthorizedHandler() UnauthorizedHandler
}Then use type-assertion in a helper function to use either the new or old behavior: func unauthorizedError(w http.ResponseWriter, r *http.Request, desc string, state string, rp RelyingParty) {
if rp, ok := rp.(HasUnauthorizedHandler); ok {
rp.UnauthorizedHandler()(w, r, desc, state)
return
}
http.Error(w, desc, http.StatusUnauthorized)
}And call that function where required: if err := trySetStateCookie(w, state, rp); err != nil {
unauthorizedError(w, r, "failed to create state cookie: "+err.Error(), state, rp)
return
}After we merged this we can create a new PR against the |
|
Sounds interesting, I was not aware of pattern. Thanks for bringing this in. |
Signed-off-by: Jan-Otto Kröpke <[email protected]>
jkroepke
force-pushed
the
rp/UnauthorizedHandler
branch
from
7fb356a to
dca7835
Compare
|
🎉 This PR is included in version 3.10.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Any callback error which results into an unauthorized status just print as plain text to the user.
I would like to enrich the user experience and offer an HTML styled error message.
My first idea was to re-use the existing
errorHandler, but it always returnshttp.StatusInternalServerError. The best possible solution would be an errorHandler which has the preferred http status as function input. However this breaks the function signature.Definition of Ready